Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arnold Operators #5841

Closed

Conversation

boberfly
Copy link
Collaborator

@boberfly boberfly commented May 2, 2024

Generally describe what this PR will do, and why it is needed

  • Adds Arnold operators support
  • Modelled almost 1:1 to ArnoldImager so this was fairly straightforward to implement
  • Following gaffer-dev I am well aware that this wasn't a priority due to Gaffer being the procedural itself
  • I have written custom operator plugins that would be good to be able to use them in GafferArnold, which is my main motivation for this PR
  • Also included a fix for ArnoldImager menu entry by calling it Arnold/Globals/Imagers, where the imager node menu would override the actual Arnold/Globals/Imager node menu option so you could only type-search for it prior to this fix - that is a separate commit in this

WIP

  • I am not too familiar with gaffer.mtd metadata and I can't seem to get the fancies to work like filename for the materialx node, or making it do nice enums for some of the nodes
  • Operator nodes have an inputs array, I replace this with an input for the UI and the array fill-in is deferred to the AiOpLink function, but I am getting warnings from Arnold eg. WARNING | linking "set_transform()" to "set_transform(input)": target parameter "input" not recognized I'm not too sure where this is coming from yet, my suspicion is it is something to do in ArnoldShader/ParameterHandler before ShaderNetworkAlgo touches it

Related issues

  • NA

Dependencies

  • NA

Breaking changes

  • I don't think this breaks anything that I am aware of, it is more enabling something in Arnold

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

…Imagers so that the actual ArnoldImager node menu option wasn't overwritten.
@boberfly
Copy link
Collaborator Author

boberfly commented May 2, 2024

Hmm how do you get around that whitespace check for ArnoldImagerUI.py I wonder, as it has 2 spaces as well

@johnhaddon
Copy link
Member

Following gaffer-dev I am well aware that this wasn't a priority due to Gaffer being the procedural itself

Sorry Alex, but I'd go a bit further and say that we're a little opposed to Arnold operators philosophically, rather than them just not just being a priority. Putting it a bit bluntly, I see operators-in-the-renderer as a crutch for certain other DCCs with more limited procedural capabilities than Gaffer. Instead of putting effort into supporting them, I would much rather put the effort into further enhancing Gaffer's procedural capabilities to the benefit of all our renderers.

While it's true that you've already done the work, a portion of the maintenance will inevitably fall on the core team, and I'd rather not accept that burden for a feature that we don't feel positive about. It's unfortunate that the feature requires changes to the core renderer backend as otherwise I would suggest hosting this as a Gaffer extension. One potential compromise might be figuring out the absolute minimum of core changes needed to allow you to do so, but at first glance that does seem to be a fair majority of the PR, so might represent an almost identical amount of support. In the future I'd suggest discussing initiatives of this magnitude up front - I hate pushing back on something after you've put the effort in already.

On a brighter note, I'd certainly be interested in learning more about your custom operator though, and putting some thought into how it might be implemented as a Gaffer feature (or a combination of features). We definitely want to push Gaffer more in the direction of procedural content creation, rather than mere assembly, so any thoughts you have along those lines would be very welcome. Thanks also for the fix to the Imagers menu - I've cherry-picked that over for the next 1.4 release already.

I'll leave this PR open for a few days to leave room for further discussion, but otherwise I'm afraid my intention is to close it...

@boberfly
Copy link
Collaborator Author

boberfly commented May 8, 2024

Hi John yep no worries I am in agreement on this one in general. I was having a chat with @murraystevenson already on a few things I can try, this was more for a 1:1 match of something already written but ultimately I am after a more generic approach that can work with all renderers.

I'll look into sharing what kind of operators I've written, which certainly can become actual Gaffer procedural nodes as an alternative approach - I think one of them might be tricky to implement.

Cheers

@johnhaddon
Copy link
Member

Hi John yep no worries I am in agreement on this one in general.

OK great, thanks for your understanding. I'll close this one then, and look forward to chatting about potentially more generic approaches...

@johnhaddon johnhaddon closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants